Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ntlm authentication #2658

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

samhanes
Copy link
Contributor

No description provided.

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow nice!
As long as CI says OK and the encoding stuff is fixed :)

I have some concerns about netcore and mono compat but in theory when CI is green it should be OK.
Regarding a x-plat paket-files NTLM is a bit of a problem, isn't it? So basically authtype: "ntlm" would render the file unusable on non-windows?

let configWithPassword = """
source http://www.nuget.org/api/v2 username: "tat� tata" password: "you got hacked!"
let configWithPasswordNoAuthType = """
source http://www.nuget.org/api/v2 username: "tat� tata" password: "you got hacked!"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something broke here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh yikes, I'll have a look!

@samhanes
Copy link
Contributor Author

samhanes commented Aug 24, 2017

Yes, I think you're right - specifying NTLM in a file would mean it can't be used on non-windows.

I had considered limiting the auth type specification to config files for that reason, but I ultimately decided that if your nuget feed requires windows auth, you're already in trouble if you're not on windows. Do you think that makes sense?

@matthid
Copy link
Member

matthid commented Aug 24, 2017

Yep, just asking for confirmation I guess :)

```

If you don't want to check your username and password into source control, you
can use environment variables instead:

```paket
source http://myserver/nuget/api/v2 username: "%PRIVATE_FEED_USER%" password: "%PRIVATE_FEED_PASS%"
source http://myserver/nuget/api/v2 username: "%PRIVATE_FEED_USER%" password: "%PRIVATE_FEED_PASS%" authtype: "ntlm"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need either username/password OR ntlm, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you need to enter your windows account and password? It should be able to use the current windows account.

Copy link
Contributor Author

@samhanes samhanes Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the default credentials aren't used - just a different authentication scheme. See my reply to your other comment. :)

@0x53A
Copy link
Contributor

0x53A commented Aug 24, 2017

So i may be misunderstanding something, I haven't looked at this too deep to be honest.

But normally I understand WindowsAuthentication to mean that the current ambient user account will be used, and I DON'T need to enter my password anywhere.

Maybe this could help? https://stackoverflow.com/a/11414695/1872399

@samhanes
Copy link
Contributor Author

@0x53A Using the default credentials is certainly one use case - but in this case we'd like to use Windows credentials to a separate domain for authentication. Locally we can store them in the windows credentials manager, but this still presents a problem on the build server. The changes I've proposed would allow you to provide a username in the form of domain\user and then paket will those credentials to authenticate the request via NTLM.

@0x53A
Copy link
Contributor

0x53A commented Aug 24, 2017

ok, makes sense.

I guess if someone wants default credentials they should implement it themselves ;D

@forki
Copy link
Member

forki commented Aug 25, 2017

WOW! This looks great!

@forki forki merged commit e012329 into fsprojects:master Aug 25, 2017
@samhanes
Copy link
Contributor Author

Thanks - you guys move fast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants